Skip to content

Improve readability of count validation in lazy-response-ads - #1228

Open
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/lazy-response-ads-validation
Open

Improve readability of count validation in lazy-response-ads#1228
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/lazy-response-ads-validation

Conversation

@pavankumar-vh

Copy link
Copy Markdown

Overview

Improve code readability by extracting a complex ternary expression into a separate variable.

Change

The original code had a nested ternary inside Math.min/Math.max:

const count = Number.isFinite(params.count)
  ? Math.min(MAX_RESPONSE_AD_POOL_SIZE, Math.max(0, Math.floor(params.count)))
  : 0

This was hard to read. Extracted the validation into a separate variable:

const safeCount = Number.isFinite(params.count) ? params.count : 0
const count = Math.min(MAX_RESPONSE_AD_POOL_SIZE, Math.max(0, Math.floor(safeCount)))

Testing

All existing tests pass (5/5).

Files Changed

  • common/src/util/lazy-response-ads.ts - Refactored count validation

Scope

This change only touches common/ which is an approved contribution area per the Contributing Guide.

The original code used a ternary inside Math.min/Math.max which was hard to read.
Extracted the Number.isFinite check into a separate variable for clarity.

This is a pure refactor with no behavioral change - the logic is identical.
@codebuff-team

Copy link
Copy Markdown
Contributor

Thanks for the contribution, but this doesn't change behavior at all — it's a pure stylistic rewrite of the same ternary/min/max logic, just moved into two lines instead of one. The original Number.isFinite(params.count) ? Math.min(...) : 0 is not meaningfully harder to read than the split version, and there's no bug being fixed, no edge case handled differently, and no test added or changed to justify the diff.

Maintainers generally won't port a change like this by hand into the private tree because the cost of reviewing/porting exceeds the value of the change - the code is functionally identical before and after. If you want to contribute to common/, look for an actual bug, a missing edge case, or a genuine clarity issue that changes how a reader reasons about correctness (e.g. a case where the ternary nesting hides a real logic error), rather than a pure formatting pass over already-correct code.

Closing as this is style-only churn with no substantive improvement.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:rejected Not a change this project wants; closed with an explanation labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:rejected Not a change this project wants; closed with an explanation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants